Fix 500s and empty results from binary fields in Calcite pushdowns - #5767
Conversation
PR Reviewer Guide 🔍(Review updated until commit 55f67f5)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 55f67f5 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit d4f1715
Suggestions up to commit 8223f3e
|
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (63.24%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #5767 +/- ##
============================================
- Coverage 63.24% 63.24% -0.01%
- Complexity 8820 8824 +4
============================================
Files 938 938
Lines 40211 40237 +26
Branches 4530 4538 +8
============================================
+ Hits 25432 25446 +14
- Misses 13957 13966 +9
- Partials 822 825 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A binary field has neither fielddata nor doc values, but the plan-time guards test whether a type is atomic rather than aggregatable, so OpenSearchBinaryType passes them and reaches the shard. Nine PPL commands fail there with a 500, and where isnotnull returns 200 with zero rows because BinaryFieldMapper writes no _field_names entry for exists. Refuse a binary reference in NamedFieldExpression.getReference and getReferenceForTermQuery, in the two field-sort sites of AbstractCalciteIndexScan, and in the AggregateAnalyzer dedup sort hint. Those pushdowns decline and Calcite returns correct rows un-pushed, while the filter path re-analyzes as a _source script. RexStandardizer routes a binary field to _source, so a pushed-down script over one now reads its real base64 value instead of null from doc values. Signed-off-by: Chayanin Noramuttha <cnoramut@gmail.com>
8223f3e to
d4f1715
Compare
|
Persistent review updated to latest commit d4f1715 |
| * top_hits} fetch field is served from {@code _source} by the fields API and so is valid. | ||
| */ | ||
| private static void rejectBinaryField(String name, ExprType type) { | ||
| if (type instanceof OpenSearchBinaryType) { |
There was a problem hiding this comment.
The binary-field guards check the raw resolved type with instanceof OpenSearchBinaryType. That misses alias fields: an alias pointing at a binary field resolves to OpenSearchAliasType, not OpenSearchBinaryType, so it slips past every guard and runs into the exact bugs this PR fixes — isnotnull(alias) silently returns 0 rows, and sort alias fails with a 500.
There was a problem hiding this comment.
Checked this on a cluster and the alias case is already covered.
You are right that the flattened type map holds OpenSearchAliasType under the alias key, and the guards are never handed that key. OpenSearchTypeFactory.convertSchema omits alias fields from the TableScan row type and CalciteRelNodeVisitor re-adds each one as a project over its target, so the name reaching fieldTypes.get is already payload and the check fires.
Measured raw beside alias, before on a29cf858a and after on this branch. The alias column is identical to the raw column on both sides, so an alias was never a distinct case here.
Measurements
| Query | raw, before | alias, before | raw, after | alias, after |
|---|---|---|---|---|
sort <field> |
500 fielddata |
500 fielddata |
4 rows | 4 rows |
where isnotnull(<field>) |
200, total: 0 |
200, total: 0 |
200, total: 4 |
200, total: 4 |
stats count() by <field> |
500 fielddata |
500 fielddata |
4 buckets | 4 buckets |
sort <field>, keyword control |
4 rows | 4 rows | 4 rows | 4 rows |
The pre-fix plan for sort payload_alias emitted "sort":[{"payload":...}], naming the real field, which a guard that only saw the alias key could not have produced.
I also checked the dedup sort hint at AggregateAnalyzer.java:630, the one site carrying a raw user-typed name into a guard. An alias never reaches it, because the project defining the alias sits between the sort and the scan and the absorption does not see through it, measured as a sort clause present in the request body for the raw field and absent for the alias.
Added six cases to issues/5757.yml covering this, with a keyword-alias control so a decline is attributable to the type. Four of them go red with the guards reverted.
graphLookup was the only caller of the three-argument PredicateAnalyzer.analyze, which hardcodes rowType and cluster to null. No ScriptQueryExpression could be built there, so the catch rethrew every unanalyzable filter as a RuntimeException. That had two consequences. The binary refusals added by the previous commit made isnotnull on a binary field unanalyzable at that site, turning a 200 into a 500. Separately every graphLookup filter needing a script was already failing on main, confirmed for n + 1 > 3, abs(n - 3) < 1, length(name) = 1 and upper(name) = 'C' while a plain n > 2 range filter succeeded. Pass rowType and cluster, both already in scope at the call. The four script filters now return correct edges, and n + 1 > 3 matches the plain n > 2 form it reduces to. Add eleven cases to issues/5757.yml over two new indices. One carries field aliases over a binary and a keyword field, since an alias resolves to its target before any pushdown site inspects the type and nothing pinned that. The other carries a graph whose node c has no payload, so isnotnull discriminates instead of matching every document. Reverting the four source files from the previous commit reddens 23 of the 30 cases, and reverting only this one reddens exactly two. Signed-off-by: Chayanin Noramuttha <cnoramut@amazon.com>
|
Persistent review updated to latest commit 55f67f5 |
|
Chasing @ahkcs's alias question led me through the other filter sites, and it turned up a regression this PR would have shipped plus a pre-existing bug behind it. Both are fixed by the same two arguments, so flagging it here since it widens the diff beyond the binary guards.
Two consequences. First, the binary guards in this PR made Second, the same omission was already breaking every So The fix passes the two arguments, both already in scope at the call. QueryBuilder filterQuery =
PredicateAnalyzer.analyze(
graphLookup.filter,
schema,
fieldTypes,
graphLookup.getLookup().getRowType(),
graphLookup.getCluster());All four now return correct edges. On scope, the binary half is not separable, since the Tests: |
Description
source=idx | sort binon abinaryfield returns a 500 whosereasonis a genericFailed to fetch data from the index. The real cause is visible only indetails, where it readsIllegalArgumentException[Can't load fielddata on [bin] because fielddata is unsupported on fields of type [binary]]. Nine commands fail this way. A tenth,where isnotnull(bin), returns 200 with zero rows against documents that all have the field populated.A
binaryfield has neither fielddata nor doc values, so OpenSearch cannot bucket or sort on it. The plan-time guards ask whether a type is atomic, not whether it is aggregatable.Binary("binary", ExprCoreType.UNKNOWN)reads as atomic, passes every guard, and reaches the shard.geo_pointbecomesGEOMETRY, fails the same test, and is already rejected as a 400 by #5751.binaryis not.The filter case fails differently, and silently.
existsQuery(bin)is valid DSL, butBinaryFieldMapperindexes nothing whendoc_valuesis false, not even a_field_namesentry, soexistshas no term to match and the shard honestly reports zero hits for a correctly-formed query.This refuses a binary reference where a field reference resolves, so each pushdown declines and Calcite keeps an un-pushed plan that returns correct results.
PredicateAnalyzer.NamedFieldExpression.getReference()andgetReferenceForTermQuery(), the seam every affected family already routes through. The filter path re-analyzes the predicate as a_sourcescript and stays pushed down, while the aggregate and sort paths decline the planner rule.AbstractCalciteIndexScan, the two field-sort sites. The script-sort branch beside them needs no guard, since it reads from_source.AggregateAnalyzer, the dedup sort hint, which carries a raw field name that never reaches the accessors above.RexStandardizer, route a binary field to_sourcerather than doc values, which is what makes the filter redirect work.Refusing in the accessors rather than in the
NamedFieldExpressionconstructors is deliberate. Atop_hitsfetch field only needsgetRootName(), andBinaryFieldMapper.BinaryFieldType.valueFetcherreturnsSourceValueFetcher.identity, so the fields API serves a binary field from_sourceand that request shape was always valid. A constructor-level refusal would declinededupon any index whose mapping merely contains a binary field, even when the query never names it, costing a pushdown that works correctly today, 0.0075s pushed down against 0.254s declined over 50000 documents. The last case in the test file pins this.Before
After, the same query, HTTP 200
Behaviour on a
binaryfield, measured on a live cluster before and after.sort binstats count() by binstats max(bin)top 2 bin,rare 2 bin,dedup bintimechart span=1m count() by bin,chart count() over m by binxyseries m bin IN ('x','y') csort bin | dedup mwhere isnotnull(bin)sort latency | fields binOne behaviour change beyond the reported symptom. The
RexStandardizerchange applies to every script context, so a pushed-down script referencing a binary field previously readnullfrom doc values and now reads the real base64 value.eval x = concat(bin, 'a')returns a value where it used to return null.That also moves where a bad script fails. A comparison against a binary field is now compiled on the shard against the real value, so
where bin = 'zzz'returns aQueryShardExceptionfor a script it cannot compile rather than the earlier fielddata error. Nothing regresses, but the error text changes.graphLookupon a binary edge field is the one caller these accessors do not protect, and it was already broken.CalciteEnumerableGraphLookup.queryLookupTableresolves the edge field at execution time outside any decline path, so the refusal escapes as a 500 instead of declining a rule. It returned a 500 before this change as well, since thetermsquery it emitted on a binary field fails at the shard, so the only difference is that the message now names the field. Verified on a live cluster, a keyword edge returns rows and a binary edge returns 500 either side of the change.graphLookupis marked experimental and a base64 blob is not a plausible graph edge, so this is recorded, not fixed.An
aliasfield whosepathis a binary field looks unguarded and is not.instanceof OpenSearchBinaryTypeis false for theOpenSearchAliasTypesuch a mapping produces, but Calcite resolves the alias at plan time to the base field's input ref, so the explain showspayload_alias=[$1]where$1ispayload, and the name reaching the guard is never the alias. Verified on a live cluster.Out of scope, noted in the issue. Comparing a binary field against a string still fails, and not only for
=.where bin = 'zzz'andwhere bin != 'zzz'reportCannot cast "java.lang.String" to "org.apache.calcite.avatica.util.ByteString",where bin > 'a'reports no applicableSqlFunctions.gtoverload, andwhere match(bin, 'zzz')cannot work at all against a field OpenSearch does not index. Every one of those fails with pushdown disabled too, so declining the pushdown exposes the coercion gap rather than causing it, and the family belongs with #5753. What this fixes on the filter side is the null predicates,isnotnull(bin)returning all populated rows instead of none andisnull(bin)returning none, which is what the test file covers.Also,
stats count() by bindeclines rather than taking the scripted_sourcerouteAggregateAnalyzeralready uses for atextfield with no.keyword. That costs a full scan, measured at 0.42s against 0.17s for a keyword group key over the same 50000 documents, so roughly 2.5x on a different cardinality rather than a like-for-like comparison. Worth it against a 500, but worth reclaiming. A blanketnullreturn is not the way, because it would makemax(bin)build atop_hitswith no sort and return an arbitrary document, so keeping that pushdown needs the value-source site separated from the sort sites.Related Issues
Resolves #5757
Testing
integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5757.yml, 19 cases over HTTP against a real binary mapping, covering all three pushdown families plus thexyseriesaggregate-filter-argument route and the dedup sort hint. 18 assert answers. The last asserts a plan, because the accessor-versus-constructor placement above returns identical correct rows either way and is invisible to an answer assertion.AggregateAnalyzerdedup-hint check fails only the dedup sort-hint case. Moving the refusal into the constructors fails only the plan case.RelJsonSerializerTest.testSerializeAndDeserializeUDTchanges one expected value, the script source for thebinaryfield, fromDOC_VALUEtoSOURCE. That flip is the unit-level assertion of theRexStandardizerchange and of the behaviour change noted above, so it is the point of the edit rather than a fixup around it.:opensearch:test:integ-test:yamlRestTest -Dtests.rest.suite=issues/5757CalciteExplainIT, pushdown and no-pushdown variantsspotlessCheckVerified against a local 3.9.0-SNAPSHOT tarball, single node and single shard, with
plugins.calcite.enabledandplugins.calcite.pushdown.enabledboth true. Not tested multi-shard or with security enabled.Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.